Sanitize ODBC sample connection string to unblock dnceng mirror - #8321
Merged
Conversation
The doc-comment in TestDataConnectionSql.Helpers.cs contained the literal substring `Pwd=password`, which Azure DevOps push protection flags as an embedded credential (VS403654), causing the GitHub -> AzDO mirror of microsoft/testfx to fail (#8291). The same comment lived in TestDataConnectionSql.cs since 2016 and had been mirrored long before push protection started flagging it. The recent refactor (#8270) re-introduced the pattern in a new file, which triggered the scanner on every subsequent fast-forward push. Replace `Uid=user;Pwd=password` with `Uid=<user>;Pwd=<password>` so the example remains useful but no longer matches the connection-string-with- credentials detector. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR sanitizes a sample ODBC connection string in a documentation comment to avoid Azure DevOps push protection treating it as an embedded credential, unblocking repository mirroring.
Changes:
- Replaces literal sample credentials with XML-escaped placeholders in the ODBC example.
- Preserves the usefulness and XML validity of the doc comment.
Show a summary per file
| File | Description |
|---|---|
src/Adapter/MSTestAdapter.PlatformServices/Data/TestDataConnectionSql.Helpers.cs |
Sanitizes the ODBC Oracle sample connection string in the KnownOdbcDrivers remarks. |
Copilot's findings
- Files reviewed: 1/1 changed files
- Comments generated: 0
Evangelink
commented
May 18, 2026
Evangelink
left a comment
Evangelink
left a comment
Member
Author
There was a problem hiding this comment.
Review Summary
This is a single-line XML doc comment fix that HTML-encodes user and password placeholders so they render correctly as <user> and <password> in generated documentation rather than being misinterpreted as XML tags.
| # | Dimension | Status | Notes |
|---|---|---|---|
| 1 | Algorithmic Correctness | ✅ N/A | No logic changes |
| 2 | Threading & Concurrency | ✅ N/A | No threading changes |
| 3 | Security & IPC Contract Safety | ✅ Clean | Encoding placeholders reduces risk of misreading as real credentials |
| 4 | Public API & Binary Compatibility | ✅ N/A | Comment-only change |
| 5 | Performance & Allocations | ✅ N/A | No runtime change |
| 6 | Cross-TFM Compatibility | ✅ N/A | No runtime change |
| 7 | Resource & IDisposable Management | ✅ N/A | No runtime change |
| 8 | Defensive Coding at Boundaries | ✅ N/A | No runtime change |
| 9 | Localization & Resources | ✅ N/A | No runtime change |
| 10 | Test Isolation | ✅ N/A | No test changes |
| 11 | Assertion Quality | ✅ N/A | No test changes |
| 12 | Flakiness Patterns | ✅ N/A | No test changes |
| 13 | Test Completeness & Coverage | ✅ N/A | Documentation-only |
| 14 | Data-Driven Test Coverage | ✅ N/A | Documentation-only |
| 15 | Code Structure & Simplification | ✅ N/A | No structural changes |
| 16 | Naming & Conventions | ✅ N/A | No naming changes |
| 17 | Documentation Accuracy | ✅ Clean | <user> and <password> correctly encode angle-bracket placeholders so XML doc tools render them as <user> and <password> rather than stripping them as unknown XML tags |
| 18 | Analyzer & Code Fix Quality | ✅ N/A | Not an analyzer change |
| 19 | IPC Wire Compatibility | ✅ N/A | No runtime change |
| 20 | Build Infrastructure & Dependencies | ✅ N/A | No build changes |
| 21 | Scope & PR Discipline | ✅ Clean | Minimal, focused fix |
Verdict: ✅ Clean documentation fix. No issues found.
Generated by Expert Code Review (on open) for issue #8321 · ● 4.3M
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8291.
Root cause
PR #8270 (
RefactorTestDataConnectionSqlinto focused partial modules) moved a long-standing doc comment containing a sample ODBC connection string into a brand-new fileTestDataConnectionSql.Helpers.cs. That comment contains the literal substringPwd=password, which Azure DevOps push protection flags as an embedded credential (VS403654 — ODBC connection string with credentials) and rejects the fast-forward mirror push from GitHub todev.azure.com/dnceng/internal/_git/microsoft-testfx.The same comment lived in
TestDataConnectionSql.cssince the 2016 initial commit and had already been mirrored long before push protection was enabled, so the original location is grandfathered in. The refactor re-introduced the pattern in a new file, which now trips the scanner on every subsequent push.Timing matches:
037e9b09epushedFix
Replace
Uid=user;Pwd=passwordwithUid=<user>;Pwd=<password>(XML-encoded as<user>/<password>so the doc-comment XML stays valid). The example remains useful and self-explanatory but no longer matches the connection-string-with-credentials detector.